-
Notifications
You must be signed in to change notification settings - Fork 3
Feature Add positions DTOs and enhance invoicing #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature Add positions DTOs and enhance invoicing #32
Conversation
20011fd
to
14fefa3
Compare
@kaspernowak |
@StanBarrows Thank you! Looking forward to it. I think this would have higher prio than #34, as the structured addresses won't really be required before December. |
@StanBarrows @RhysLees do you have an estimate of when you would have time to take a closer look at this? |
@kaspernowak It’s quite a big refactor, and we want to do it properly. I’ve planned around 2–3 days to review, adjust, and test it thoroughly. But if that’s too long for you, feel free to create a fork and merge it there in the meantime. |
@StanBarrows Thank you! Hopefully we won't need this before you have time to look at it, but otherwise we will run this from a fork. Do you mean this is a big refactor due to that apps you support currently use the current invoice approach? I feel like this PR alone adds more additional features than it affects existing functionality, and therefore I also feel like this is a much smaller refactor than the OAuth2 PR, but I might be wrong. |
As far as I know, your PR was built as an addition so that we can support both methods. I’d first like to review the current Bexio API documentation myself, since we developed this approach some time ago, and then decide whether to keep both or drop the existing implementation. It would also make sense to check if any other updates are needed for other objects, as there are still open PRs that involve further API changes. I'm just not updated about any changes to the Bexio API the last couple of years. After that, Rhys will likely review your entire PR and process it and then update the documentation accordingly. All in all, I need to set aside time to properly look into this — and after that, it’s up to Rhys. With a major go-live coming up in early August, even finding a few hours a day is currently not feasible. P.S. The OAuth integration — even with your PR as a reference — still took about two full working days to reach its current state. So the current PR is maybe not more complex in a technical way, but it involves more research and coordination with other people, which makes it feel more complex to me. And yeah, then I’ll also need to update all the projects where I’ve implemented the current state, to avoid technical debt. And since this isn’t covered by any customer budget, I’ll have to fit it into time slots where I have some capacity. |
@StanBarrows I completely understand, thank you for the explanation. |
This PR introduces complete support for creating and sending invoices using structured DTOs, including all supported Bexio position types.
✅ What’s Included
AddDefaultPositionDTO
AddTextPositionDTO
src/Dto/*
CreateAnInvoiceRequestNew
EditAnInvoiceRequestNew
SendAnInvoiceRequest
MarkAnInvoiceAsSentRequest
SendInvoiceDTO
💡 Usage Example
The existing
CreateAnInvoiceRequest
class appears to be incompatible with the documented example due to a type mismatch in thefilterPositions()
method:To avoid breaking changes, I've added new request classes (
CreateAnInvoiceRequestNew
, etc.) that work with the new DTO structure.We may want to decide whether to:
closes #7